Remove ill-conceived concept of watches blocking reply on

connection which did write/mkdir/rm/setperm etc.
This causes deadlocks in real life, and I can't see a sane way
of avoiding them: it is reasonable for someone to ignore watch
notifications while doing other actions, and that means that
we can do other writes.  These writes can block pending other
watchers; if one of these is the process blocked awaiting our
ack, we deadlock.
Signed-off-by: Rusty Russel <rusty@rustcorp.com.au>
Signed-off-by: Christian Limpach <Christian.Limpach@cl.cam.ac.uk>

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 3bf63314b75f5e3623f26c32b1b64f8e5362f27e..e257853cb2b8bc94474bf272fbaa36bacbdd6e97 100644 (file)
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -976,10 +976,7 @@ static void do_write(struct connection *conn, struct buffered_data *in)
        }
 
        add_change_node(conn->transaction, node, false);
-       if (fire_watches(conn, node, false)) {
-               conn->watch_ack = XS_WRITE;
-               return;
-       }
+       fire_watches(conn, node, false);
        send_ack(conn, XS_WRITE);
 }
 
@@ -1005,10 +1002,7 @@ static void do_mkdir(struct connection *conn, const char *node)
        }
 
        add_change_node(conn->transaction, node, false);
-       if (fire_watches(conn, node, false)) {
-               conn->watch_ack = XS_MKDIR;
-               return;
-       }
+       fire_watches(conn, node, false);
        send_ack(conn, XS_MKDIR);
 }
 
@@ -1046,10 +1040,7 @@ static void do_rm(struct connection *conn, const char *node)
        }
 
        add_change_node(conn->transaction, node, true);
-       if (fire_watches(conn, node, true)) {
-               conn->watch_ack = XS_RM;
-               return;
-       }
+       fire_watches(conn, node, true);
        send_ack(conn, XS_RM);
 }
 
@@ -1121,10 +1112,7 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in)
        }
 
        add_change_node(conn->transaction, node, false);
-       if (fire_watches(conn, node, false)) {
-               conn->watch_ack = XS_SET_PERMS;
-               return;
-       }
+       fire_watches(conn, node, false);
        send_ack(conn, XS_SET_PERMS);
 }
 
@@ -1359,12 +1347,6 @@ static void unblock_connections(void)
                                consider_message(i);
                        }
                        break;
-               case WATCHED:
-                       if (i->watches_unacked == 0) {
-                               i->state = OK;
-                               send_ack(i, i->watch_ack);
-                       }
-                       break;
                case OK:
                        break;
                }
@@ -1389,8 +1371,6 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read)
 
        new->state = OK;
        new->blocked_by = NULL;
-       new->watch_ack = XS_ERROR;
-       new->watches_unacked = 0;
        new->out = new->waiting_reply = NULL;
        new->fd = -1;
        new->id = 0;
@@ -1471,7 +1451,6 @@ void dump_connection(void)
                printf("    state = %s\n",
                       i->state == OK ? "OK"
                       : i->state == BLOCKED ? "BLOCKED"
-                      : i->state == WATCHED ? "WATCHED"
                       : "INVALID");
                if (i->id)
                        printf("    id = %i\n", i->id);

diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 9643d36b9572cae7ce82c20e9db4373f85c7b6b4..22662a2eb7cb6b3314bf1beda09d0652746dd53c 100644 (file)
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -51,8 +51,6 @@ enum state
 {
        /* Blocked by transaction. */
        BLOCKED,
-       /* Waiting for watchers to ack event we caused */
-       WATCHED,
        /* Completed */
        OK,
 };
@@ -73,12 +71,6 @@ struct connection
        /* Node we are waiting for (if state == BLOCKED) */
        char *blocked_by;
 
-       /* Are we waiting for watches to be acked from an event we caused? */
-       unsigned int watches_unacked;
-
-       /* Type of ack to send once watches fired. */
-       enum xsd_sockmsg_type watch_ack;
-
        /* Is this a read-only connection? */
        bool can_write;
 

diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index afaef1bef2f935dd7acf72e3414e9e03cbb101d2..eef4d29038cdb6b2d11b714d5928c5b338f23917 100644 (file)
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -307,7 +307,6 @@ void do_transaction_end(struct connection *conn, const char *arg)
 {
        struct changed_node *i;
        struct transaction *trans;
-       bool fired = false;
 
        if (!arg || (!streq(arg, "T") && !streq(arg, "F"))) {
                send_error(conn, EINVAL);
@@ -337,12 +336,8 @@ void do_transaction_end(struct connection *conn, const char *arg)
 
                /* Fire off the watches for everything that changed. */
                list_for_each_entry(i, &trans->changes, list)
-                       fired |= fire_watches(conn, i->node, i->recurse);
+                       fire_watches(conn, i->node, i->recurse);
        }
-
-       if (fired)
-               conn->watch_ack = XS_TRANSACTION_END;
-       else
-               send_ack(conn, XS_TRANSACTION_END);
+       send_ack(conn, XS_TRANSACTION_END);
 }
 

diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c
index a49dcbc954b007fd658e29e82fc9510f93db31ca..1c42b72cedfbd1f73fa76dcb1a7934d9668c413a 100644 (file)
--- a/tools/xenstore/xenstored_watch.c
+++ b/tools/xenstore/xenstored_watch.c
@@ -41,9 +41,6 @@ struct watch_event
        /* Data to send (node\0token\0). */
        unsigned int len;
        char *data;
-
-       /* Connection which caused watch event (which we are blocking) */
-       struct connection *cause;
 };
 
 struct watch
@@ -95,14 +92,10 @@ static int destroy_watch_event(void *_event)
        struct watch_event *event = _event;
 
        trace_destroy(event, "watch_event");
-       assert(event->cause->watches_unacked != 0);
-       /* If it hits zero, will unblock in unblock_connections. */
-       event->cause->watches_unacked--;
        return 0;
 }
 
-static void add_event(struct connection *cause, struct watch *watch,
-                     const char *node)
+static void add_event(struct watch *watch, const char *node)
 {
        struct watch_event *event;
 
@@ -117,22 +110,20 @@ static void add_event(struct connection *cause, struct watch *watch,
        event->data = talloc_array(event, char, event->len);
        strcpy(event->data, node);
        strcpy(event->data + strlen(node) + 1, watch->token);
-       event->cause = cause;
-       cause->watches_unacked++;
        talloc_set_destructor(event, destroy_watch_event);
        list_add_tail(&event->list, &watch->events);
        trace_create(event, "watch_event");
 }
 
 /* FIXME: we fail to fire on out of memory.  Should drop connections. */
-bool fire_watches(struct connection *conn, const char *node, bool recurse)
+void fire_watches(struct connection *conn, const char *node, bool recurse)
 {
        struct connection *i;
        struct watch *watch;
 
        /* During transactions, don't fire watches. */
        if (conn->transaction)
-               return false;
+               return;
 
        /* Create an event for each watch.  Don't send to self. */
        list_for_each_entry(i, &connections, list) {
@@ -141,18 +132,16 @@ bool fire_watches(struct connection *conn, const char *node, bool recurse)
 
                list_for_each_entry(watch, &i->watches, list) {
                        if (is_child(node, watch->node))
-                               add_event(conn, watch, node);
+                               add_event(watch, node);
                        else if (recurse && is_child(watch->node, node))
-                               add_event(conn, watch, watch->node);
+                               add_event(watch, watch->node);
                        else
                                continue;
-                       conn->state = WATCHED;
                        /* If connection not doing anything, queue this. */
                        if (!i->out)
                                queue_next_event(i);
                }
        }
-       return conn->state == WATCHED;
 }
 
 static int destroy_watch(void *_watch)

diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h
index d1fac70502c32b6147640d27e6697b2e46789cc8..30d7e9e1726e3dfa71e6b24c0ed09b4070dc1776 100644 (file)
--- a/tools/xenstore/xenstored_watch.h
+++ b/tools/xenstore/xenstored_watch.h
@@ -33,9 +33,8 @@ bool is_watch_event(struct connection *conn, struct buffered_data *out);
 void queue_next_event(struct connection *conn);
 
 /* Fire all watches: recurse means all the children are effected (ie. rm).
- * Returns true if there were any, meaning connection has to wait.
  */
-bool fire_watches(struct connection *conn, const char *node, bool recurse);
+void fire_watches(struct connection *conn, const char *node, bool recurse);
 
 /* Find shortest timeout: if any, reduce tv (may already be set). */
 void shortest_watch_ack_timeout(struct timeval *tv);